Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds branch-aware functionality across the codebase: branch-normalized filenames/parsers, BranchManifestMetadata and BranchFileMapping, branch-aware IO/Cloud/GC/replayer plumbing, new branch requests (CreateBranch, DeleteBranch, GlobalCreateBranch), and changes EloqStore::Start to accept a branch and term. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EloqStore
participant Shard
participant BackgroundWrite
participant AsyncIoManager
participant CloudStoreMgr
Client->>EloqStore: GlobalCreateBranchRequest(branch, parent)
EloqStore->>EloqStore: HandleGlobalCreateBranchRequest()
EloqStore->>Shard: enqueue per-partition CreateBranchRequest
Shard->>BackgroundWrite: StartTask(CreateBranchRequest)
BackgroundWrite->>AsyncIoManager: SetActiveBranch(branch)
BackgroundWrite->>AsyncIoManager: WriteBranchManifest(branch_metadata)
AsyncIoManager->>CloudStoreMgr: Upload branch manifest/data
CloudStoreMgr-->>BackgroundWrite: ACK / result
BackgroundWrite-->>Shard: task completion
Shard-->>EloqStore: aggregate result
EloqStore-->>Client: result_branch / status
sequenceDiagram
participant GC
participant FileGC
participant Replayer
participant AsyncIoManager
participant CloudStoreMgr
GC->>FileGC: ExecuteLocalGC()
FileGC->>FileGC: ClassifyFiles() -> manifest_branch_names, archive_branch_names
FileGC->>FileGC: AugmentRetainedFilesFromBranchManifests(manifest_branch_names)
loop per-branch
FileGC->>Replayer: ReplayManifest(branch_manifest)
Replayer->>AsyncIoManager: GetBranchFileMapping(tbl)
AsyncIoManager-->>Replayer: branch file_ranges
Replayer-->>FileGC: retained_files
end
FileGC->>CloudStoreMgr: DeleteUnreferencedCloudFiles(per-branch max_file_id map)
CloudStoreMgr-->>FileGC: done
FileGC-->>GC: GC complete
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (6)
src/tasks/write_task.cpp (1)
379-388: Consider clearer variable names.The variables
unused_branchandunused_termare misleading since they are actually used—specifically to check whether a mapping exists viaGetBranchNameAndTerm. Consider renaming them toexisting_branchandexisting_term, or simply use a dedicatedHasBranchMapping()method if available.💡 Suggested naming improvement
- std::string unused_branch; - uint64_t unused_term; - if (!IoMgr()->GetBranchNameAndTerm( - tbl_ident_, file_id_before_allocate, unused_branch, unused_term)) + std::string existing_branch; + uint64_t existing_term; + if (!IoMgr()->GetBranchNameAndTerm( + tbl_ident_, file_id_before_allocate, existing_branch, existing_term))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tasks/write_task.cpp` around lines 379 - 388, The variables unused_branch and unused_term are misleading because they are inspected by IoMgr()->GetBranchNameAndTerm; rename them to existing_branch and existing_term (or existingBranch/existingTerm per project naming) to reflect their actual role, update all references in the block where GetBranchNameAndTerm, SetBranchFileIdTerm, file_id_before_allocate and file_id_term_mapping_dirty_ are used, and optionally replace the call with a clearer helper like HasBranchMapping(tbl_ident_, file_id_before_allocate) if such a method exists to encapsulate the boolean check.tests/common.h (1)
60-60: Centralize the default S3 endpoint to avoid config drift.The same endpoint string appears in multiple places; consider using one shared constant/source for all defaults.
Also applies to: 74-74, 105-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/common.h` at line 60, Replace repeated literal "http://store-1:9000" with a single shared constant to avoid drift: add a header-level constant (e.g., constexpr const char* DEFAULT_S3_ENDPOINT or static const std::string DEFAULT_S3_ENDPOINT) in tests/common.h and update the struct initializers that currently set .cloud_endpoint = "http://store-1:9000" (and the other occurrences noted) to use DEFAULT_S3_ENDPOINT instead.tests/branch_gc.cpp (1)
290-299: Hardcoded manifest filename may be fragile.The test constructs the manifest filename as
"manifest_corrupt_0"based on the assumption that branch name "corrupt" normalizes to "corrupt" and term is 0. Consider usingBranchManifestFileName("corrupt", 0)to ensure consistency with the actual filename generation logic:♻️ Use filename generation helper for consistency
fs::path table_path = fs::path(branch_gc_opts.store_path[0]) / bgc_tbl_id.ToString(); - fs::path corrupt_manifest = table_path / "manifest_corrupt_0"; + fs::path corrupt_manifest = + table_path / eloqstore::BranchManifestFileName("corrupt", 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/branch_gc.cpp` around lines 290 - 299, The test currently hardcodes the manifest filename "manifest_corrupt_0", which is brittle; replace that construction with the project's filename helper by building corrupt_manifest as table_path / BranchManifestFileName("corrupt", 0) so the test uses the same normalization/formatting as production; update the code that declares corrupt_manifest (and any related expectations) to call BranchManifestFileName("corrupt", 0) and remove the hardcoded string.src/eloq_store.cpp (1)
1163-1168: Re-enqueue on task pool exhaustion for consistency with other branch request types.When
ExecAsynfails, the request is marked asNotRunningbut not re-enqueued for retry. This differs from the handling inShard::OnReceivedReq(lines 359-373) whereCreateBranch/DeleteBranchrequests are re-enqueued whenProcessReqreturns false.Consider whether failed per-partition requests should also be retried rather than immediately failing the global operation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/eloq_store.cpp` around lines 1163 - 1168, The ExecAsyn failure branch currently marks the per-partition request (ptr) as NotRunning and gives up; change it to re-enqueue the request for retry like Shard::OnReceivedReq does for CreateBranch/DeleteBranch: detect ExecAsyn returning false in the block around ExecAsyn(ptr, 0, on_branch_done), do not call ptr->SetDone(KvError::NotRunning) immediately but instead push the request back onto the same task queue or call the existing enqueue/ProcessReq-style helper used for CreateBranch/DeleteBranch retries so the request will be retried; keep the on_branch_done callback and only set ptr->SetDone on final failure after retry limits are exhausted.src/storage/shard.cpp (1)
534-543: Verify intent: empty batch returns true without starting task.The refactored code returns
true(success) for empty batches without starting a task. This changes the behavior - previously, would an empty batch have been processed differently?If this is intentional (no-op for empty batches), consider adding a brief comment to clarify.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/storage/shard.cpp` around lines 534 - 543, The code currently returns success for an empty BatchWriteRequest (write_req->batch_.empty()) without starting a task, changing previous behavior; either restore the prior behavior by ensuring a task is started for empty batches (i.e., call task->SetBatch(...) or StartTask(task, req, [task](){ return task->Apply(); }) for empty batches) or, if the no-op early-return is intentional, add a brief clarifying comment above the empty-check mentioning that empty batches are treated as a no-op and thus intentionally do not start a task (referencing BatchWriteRequest, write_req->batch_, task->SetBatch, StartTask, and task->Apply).src/async_io_manager.cpp (1)
4678-4689: Delete tasks are not aggregated before waiting; each task waits individually.The delete tasks are submitted and waited one at a time inside the loop (line 4688-4689), which is inefficient. Unlike the pattern used elsewhere (e.g., batched unlink in
FileCleaner::Run), this serializes cloud delete operations.Consider batching the delete task submissions and waiting once at the end for better parallelism:
♻️ Suggested refactor
for (const std::string &path : paths_to_delete) { delete_tasks.emplace_back(path); delete_tasks.back().SetKvTask(current_task); AcquireCloudSlot(current_task); obj_store_.SubmitTask(&delete_tasks.back(), shard); - current_task->WaitIo(); } + current_task->WaitIo(); // Wait for all delete tasks🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/async_io_manager.cpp` around lines 4678 - 4689, The loop currently submits each ObjectStore::DeleteTask and immediately calls current_task->WaitIo(), serializing deletes; instead, build and submit all delete tasks first and call current_task->WaitIo() once after the loop to allow parallel cloud deletes. Specifically, in the block using KvTask* current_task, std::vector<ObjectStore::DeleteTask> delete_tasks, AcquireCloudSlot, and obj_store_.SubmitTask, move the WaitIo call out of the for-loop: for each path create delete_tasks.emplace_back(path), call delete_tasks.back().SetKvTask(current_task), AcquireCloudSlot(current_task), and obj_store_.SubmitTask(&delete_tasks.back(), shard); after the loop call current_task->WaitIo() once (ensuring delete_tasks remains in scope until then) so all submissions run in parallel rather than serially.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Line 50: Remove the ignore entry for ".pre-commit-config.yaml" from the
.gitignore so that the repository tracks the pre-commit config; locate the line
containing ".pre-commit-config.yaml" in the .gitignore and delete it, then
add/commit the now-unignored ".pre-commit-config.yaml" file to source control to
ensure consistent hooks for all contributors.
In `@benchmark/simple_bench.cpp`:
- Line 583: The call to store.Start("main", 0) ignores its return status causing
the benchmark to continue with an uninitialized store; update the code around
the call to check the boolean/Status return from Store::Start (store.Start) and
handle failures before spawning workload threads: if Start indicates failure,
log an error (or return/exit) and avoid running the benchmark, ensuring any
cleanup runs as needed; apply the same pattern used in
db_stress/concurrent_test.cpp for consistent error handling.
In `@db_stress/concurrent_test.cpp`:
- Line 72: The call to store.Start("main", 0) ignores its return status; modify
the startup in concurrent_test.cpp to check the boolean/Status returned by
Store::Start (store.Start) and abort/terminate (e.g., use CHECK or process exit)
if it fails so the stress test never runs against an uninitialized store; mirror
the validation style used in db_stress_driver.cpp and include a clear log
message referencing store.Start failure when failing fast.
In `@include/error.h`:
- Around line 32-33: Add a new C enum variant CEloqStoreStatus_AlreadyExists to
the CEloqStoreStatus enum and update the kv_error_to_c() converter to map
KvError::AlreadyExists to CEloqStoreStatus_AlreadyExists; specifically, add the
enum constant alongside the other status values and in kv_error_to_c() add a
case/branch that returns CEloqStoreStatus_AlreadyExists when the incoming kv
error is KvError::AlreadyExists so FFI callers no longer get the generic
CEloqStoreStatus_InvalidArgs for this condition.
In `@src/async_io_manager.cpp`:
- Around line 2400-2408: The loop building unlink requests must set the
IOSQE_FIXED_FILE flag when the directory FD is registered; check
dir_fd.FdPair().second (the registered bool) alongside dir_fd_int and after
obtaining io_uring_sqe* from GetSQE(UserDataType::BaseReq, &reqs.back()) set the
SQE flag (e.g., sqe->flags |= IOSQE_FIXED_FILE or call the appropriate helper)
before calling io_uring_prep_unlinkat, so registered dir_fds use the fixed-file
flag while unregistered ones do not.
- Around line 2383-2389: The code currently does
files_to_delete.reserve(max_term + 2) and iterates t=0..max_term which can
allocate/loop extremely large if max_term is corrupted; add a sanity check for
max_term before reserving/looping (e.g., compare to a configurable
MAX_TERMS_TO_DELETE or a reasonable upper bound and either clamp it or return an
error), then only reserve and push BranchManifestFileName(branch_name, t) up to
that safe_limit and finally push_back current_term_filename; apply this check
around the use of max_term in the files_to_delete logic to prevent huge
allocations/iterations.
In `@src/eloq_store.cpp`:
- Around line 1132-1154: The on_branch_done callback currently records any
non-NoError (including KvError::AlreadyExists) into req->first_error_, causing a
global failure when a partition returns AlreadyExists; change the logic in
on_branch_done to treat AlreadyExists as success by skipping the
compare_exchange_strong write when sub_err == KvError::AlreadyExists (i.e., only
attempt to set req->first_error_ if sub_err != KvError::NoError && sub_err !=
KvError::AlreadyExists), leaving the pending_ decrement and final SetDone
behavior unchanged so final_err reflects NoError when all branches are either
successful or AlreadyExists.
In `@src/file_gc.cpp`:
- Around line 676-702: The current code selects the "current" manifest solely by
matching manifest_terms[i] == process_term, which can pick the wrong branch when
multiple branches share a term; change the selection to match the actual active
branch as well: obtain the active branch name for the table (the same source
used elsewhere to determine which branch is active for tbl_id) and then search
for an index i where manifest_branch_names[i] == active_branch_name &&
manifest_terms[i] == process_term before adding
BranchManifestFileName(current_manifest_branch, process_term) to
files_to_delete; apply the same branch-aware selection fix to the analogous
block referenced at lines 710-742 so you never pick a manifest by term alone.
- Around line 382-389: The current handling in ProcessOneManifest (and similar
blocks around the other ranges) swallows replay/read errors and continues,
allowing GC to run with incomplete retained state; change these paths to
return/propagate an error from AugmentRetainedFilesFromBranchManifests() instead
of logging and continuing. Specifically, when replayer.Replay(&manifest) (and
the archive-read branches) fails, return a KvError (or suitable error code) up
the call stack rather than continuing; update ProcessOneManifest, the error
branches at the other noted ranges (449-457, 487-495), and the caller of
AugmentRetainedFilesFromBranchManifests() to check that error and abort the GC
cycle when non-NoError is returned. Ensure log messages remain but convert the
function flow to error propagation so the GC skip logic is driven by the
propagated error.
In `@src/storage/index_page_manager.cpp`:
- Around line 484-488: The code only updates branch file mappings when
replayer.branch_metadata_.file_ranges is non-empty, leaving stale mappings when
the new manifest has no ranges; change the logic in the block that currently
calls IoMgr()->SetBranchFileMapping(entry->tbl_id_,
replayer.branch_metadata_.file_ranges) so that SetBranchFileMapping is invoked
unconditionally (or call a ClearBranchFileMapping if that API exists) with
replayer.branch_metadata_.file_ranges (which may be empty) to explicitly clear
old mappings for entry->tbl_id_ whenever the manifest has no ranges.
- Around line 421-430: When GetBranchNameAndTerm(tbl_ident, max_file_id,
branch_name, term) fails we must fall back to the active branch instead of
hard-coding MainBranchName; replace the branch_name = MainBranchName assignment
with a call that returns the currently active branch (e.g. branch_name =
IoMgr()->GetActiveBranchName() or IoMgr()->ActiveBranchName()) and keep term =
IoMgr()->ProcessTerm(); ensure cloud_mgr->DownloadFile(...) is then called with
that active branch_name and term so prefetch targets the correct branch for
non-main restores.
In `@src/tasks/background_write.cpp`:
- Around line 352-360: The code currently checks BranchBaseNameExists with
UnsaltBranchName(normalized_branch) but still reuses the parent’s current file
id, which can collide with orphaned data_<file_id>_... files from a previously
deleted branch; update the branch-creation path so that if
IoMgr()->BranchBaseNameExists(...) is true (i.e., the unsalted name existed
before), you force allocation of a fresh data file id sequence for the new
branch instead of reusing the parent’s current id—do this by advancing or
allocating a new file id from the IO manager (looping/bumping the generator
until no existing data_<file_id>_* files are present) before any writes; apply
the same change to the other creation site around the 398-405 logic; reference
functions/variables: BranchBaseNameExists, UnsaltBranchName(normalized_branch),
DeleteBranch, and IoMgr() to locate and modify the logic.
- Around line 418-429: The code calls IoMgr()->WriteBranchManifest(...) then
IoMgr()->WriteBranchCurrentTerm(...), leaving a half-created branch if the
second call fails; change this to perform an atomic publish or a safe rollback:
either (preferred) write both manifest and CURRENT_TERM to temporary files and
then atomically rename/finalize them so both become visible together, or if
temp+rename helpers are not available, after WriteBranchManifest succeeds but
WriteBranchCurrentTerm fails, call the IoMgr rollback/removal API (e.g.,
IoMgr()->RemoveBranchManifest(tbl_ident_, normalized_branch) or a dedicated
Delete/Abort method) to delete the written manifest, log any rollback error
without masking the original error, and return the original KvError from
WriteBranchCurrentTerm; update the code around WriteBranchManifest and
WriteBranchCurrentTerm to use the temp+finalize or rollback APIs and ensure all
errors are propagated.
- Around line 390-401: Guard against a null mapper before dereferencing
meta->mapper_: after obtaining RootMeta* meta = root_handle.Get(), add a check
for if (!meta->mapper_) { /* partition is a stub, treat as empty */ return
KvError::NoError; } so the subsequent use of
meta->mapper_->FilePgAllocator()->CurrentFileId() is safe; reference RootMeta,
root_handle.Get(), meta->mapper_, FilePgAllocator(), and CurrentFileId() when
locating the code to update.
In `@tests/cloud_term.cpp`:
- Line 27: The tests call store->Start(eloqstore::MainBranchName, ...) but
ignore its return value; update each Start call (including the ones at the other
noted locations) to assert the outcome explicitly using the test framework
(e.g., ASSERT_TRUE/EXPECT_TRUE or ASSERT_FALSE/EXPECT_EQ) so the restart
contract is verified rather than letting a failed start manifest later;
specifically, replace the bare calls to store->Start(...) with assertions that
the return matches the expected success/failure for that scenario (use
store->Start and eloqstore::MainBranchName to locate the calls and assert true
for successful restarts and false (or the appropriate expected value) for
stale-term/failure cases).
In `@tests/cloud.cpp`:
- Line 767: The call to store->Start("main", 0) (and the other Start calls) may
fail silently causing later data assertions to report misleading mismatches;
update each occurrence (e.g., the call to store->Start in tests/cloud.cpp) to
check the return value or status and assert/signal test failure immediately if
Start() did not succeed before proceeding to dataset checks, e.g., call Start
and assert its result is OK (or throw/FAIL the test) so the test stops with a
clear startup error rather than later data validation failures.
In `@tests/eloq_store_test.cpp`:
- Line 200: Add an assertion to verify the second Start() call succeeded: after
the existing auto err2 = store.Start("main", 0); line, add REQUIRE(err2 ==
eloqstore::KvError::NoError); to assert the repeated call on store::Start
behaves as expected (references: store, Start, err2,
eloqstore::KvError::NoError).
In `@tests/gc.cpp`:
- Around line 86-93: The current check only special-cases the single-file case
for CURRENT_TERM markers; instead, filter out all branch CURRENT_TERM markers
from cloud_files before counting so partitions that contain only multiple
CURRENT_TERM.<branch> files are treated as empty. Update the logic that
builds/checks cloud_files to remove any entry matching the CURRENT_TERM marker
(e.g., filenames that start with the CURRENT_TERM. prefix or otherwise identify
as branch current-term files) — use the same naming helper used elsewhere (e.g.,
eloqstore::BranchCurrentTermFileName / the CURRENT_TERM.<branch> pattern) to
detect and exclude those entries, then base the size check on the filtered list.
In `@tests/manifest.cpp`:
- Line 343: The test currently calls store->Start(eloqstore::MainBranchName, 0)
and only checks for recovery failure after reopening; change the tests to assert
failure (or detect error) at the Start() call itself by validating Start's
return status or catching its exception right there (i.e., replace the silent
call to store->Start(...) with an assertion that Start indicates failure), and
apply the same change to the other identical occurrences of store->Start(...) in
these tests so recovery failures are detected at restart time rather than after
reopening.
In `@tests/persist.cpp`:
- Line 65: The call to store->Start(eloqstore::MainBranchName, 0) ignores its
return value so reopen failures surface later as misleading validation errors;
change the restart loop to check the boolean result of Start (or use the
existing start_store() pattern) and fail the test immediately on false (e.g.,
ASSERT/EXPECT/throw/abort used in this test suite) before proceeding to
read/validate persisted state, updating any other similar sites (e.g., the other
occurrence at line 423) to the same strict check.
---
Nitpick comments:
In `@src/async_io_manager.cpp`:
- Around line 4678-4689: The loop currently submits each ObjectStore::DeleteTask
and immediately calls current_task->WaitIo(), serializing deletes; instead,
build and submit all delete tasks first and call current_task->WaitIo() once
after the loop to allow parallel cloud deletes. Specifically, in the block using
KvTask* current_task, std::vector<ObjectStore::DeleteTask> delete_tasks,
AcquireCloudSlot, and obj_store_.SubmitTask, move the WaitIo call out of the
for-loop: for each path create delete_tasks.emplace_back(path), call
delete_tasks.back().SetKvTask(current_task), AcquireCloudSlot(current_task), and
obj_store_.SubmitTask(&delete_tasks.back(), shard); after the loop call
current_task->WaitIo() once (ensuring delete_tasks remains in scope until then)
so all submissions run in parallel rather than serially.
In `@src/eloq_store.cpp`:
- Around line 1163-1168: The ExecAsyn failure branch currently marks the
per-partition request (ptr) as NotRunning and gives up; change it to re-enqueue
the request for retry like Shard::OnReceivedReq does for
CreateBranch/DeleteBranch: detect ExecAsyn returning false in the block around
ExecAsyn(ptr, 0, on_branch_done), do not call ptr->SetDone(KvError::NotRunning)
immediately but instead push the request back onto the same task queue or call
the existing enqueue/ProcessReq-style helper used for CreateBranch/DeleteBranch
retries so the request will be retried; keep the on_branch_done callback and
only set ptr->SetDone on final failure after retry limits are exhausted.
In `@src/storage/shard.cpp`:
- Around line 534-543: The code currently returns success for an empty
BatchWriteRequest (write_req->batch_.empty()) without starting a task, changing
previous behavior; either restore the prior behavior by ensuring a task is
started for empty batches (i.e., call task->SetBatch(...) or StartTask(task,
req, [task](){ return task->Apply(); }) for empty batches) or, if the no-op
early-return is intentional, add a brief clarifying comment above the
empty-check mentioning that empty batches are treated as a no-op and thus
intentionally do not start a task (referencing BatchWriteRequest,
write_req->batch_, task->SetBatch, StartTask, and task->Apply).
In `@src/tasks/write_task.cpp`:
- Around line 379-388: The variables unused_branch and unused_term are
misleading because they are inspected by IoMgr()->GetBranchNameAndTerm; rename
them to existing_branch and existing_term (or existingBranch/existingTerm per
project naming) to reflect their actual role, update all references in the block
where GetBranchNameAndTerm, SetBranchFileIdTerm, file_id_before_allocate and
file_id_term_mapping_dirty_ are used, and optionally replace the call with a
clearer helper like HasBranchMapping(tbl_ident_, file_id_before_allocate) if
such a method exists to encapsulate the boolean check.
In `@tests/branch_gc.cpp`:
- Around line 290-299: The test currently hardcodes the manifest filename
"manifest_corrupt_0", which is brittle; replace that construction with the
project's filename helper by building corrupt_manifest as table_path /
BranchManifestFileName("corrupt", 0) so the test uses the same
normalization/formatting as production; update the code that declares
corrupt_manifest (and any related expectations) to call
BranchManifestFileName("corrupt", 0) and remove the hardcoded string.
In `@tests/common.h`:
- Line 60: Replace repeated literal "http://store-1:9000" with a single shared
constant to avoid drift: add a header-level constant (e.g., constexpr const
char* DEFAULT_S3_ENDPOINT or static const std::string DEFAULT_S3_ENDPOINT) in
tests/common.h and update the struct initializers that currently set
.cloud_endpoint = "http://store-1:9000" (and the other occurrences noted) to use
DEFAULT_S3_ENDPOINT instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dfced257-a657-4f70-8fc5-bac80d7885a6
📒 Files selected for processing (49)
.gitignorebenchmark/eloq_store_bm.ccbenchmark/load_bench.cppbenchmark/simple_bench.cppbenchmark/simple_test.cppdb_stress/concurrent_test.cppdb_stress/db_stress_driver.cppdb_stress/test_client.cppexamples/basic_example.cppinclude/async_io_manager.hinclude/common.hinclude/eloq_store.hinclude/error.hinclude/file_gc.hinclude/replayer.hinclude/storage/page_mapper.hinclude/storage/root_meta.hinclude/tasks/background_write.hinclude/tasks/prewarm_task.hinclude/types.hrust/eloqstore-sys/vendor/ffi/src/eloqstore_capi.cppsrc/async_io_manager.cppsrc/eloq_store.cppsrc/file_gc.cppsrc/replayer.cppsrc/storage/index_page_manager.cppsrc/storage/page_mapper.cppsrc/storage/root_meta.cppsrc/storage/shard.cppsrc/tasks/background_write.cppsrc/tasks/prewarm_task.cppsrc/tasks/write_task.cppsrc/test_utils.cpptests/CMakeLists.txttests/branch_filename_parsing.cpptests/branch_gc.cpptests/branch_operations.cpptests/chore.cpptests/cloud.cpptests/cloud_term.cpptests/common.cpptests/common.htests/eloq_store_test.cpptests/filename_parsing.cpptests/gc.cpptests/manifest.cpptests/manifest_payload.cpptests/persist.cpptests/replayer_term.cpp
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/async_io_manager.cpp (1)
3853-3875:⚠️ Potential issue | 🔴 CriticalFilter fallback manifest candidates by active branch before choosing max term.
At Line 3856 and Line 4105 you parse
branch_namebut selection uses only max term globally. With multiple branches, fallback can pick another branch’s manifest.🔧 Suggested fix
- for (const std::string &name : cloud_files) + const std::string_view active_branch = GetActiveBranch(); + for (const std::string &name : cloud_files) { std::string_view branch_name; uint64_t term = 0; std::optional<uint64_t> ts; if (!ParseManifestFileSuffix(name, branch_name, term, ts)) @@ if (ts.has_value()) { continue; } + if (branch_name != active_branch) + { + continue; + } if (term >= best_term) { found = true; best_term = term; selected_branch = std::string(branch_name); } }Apply the same branch filter in both fallback loops (
GetManifestandRefreshManifest).Also applies to: 4095-4120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/async_io_manager.cpp` around lines 3853 - 3875, The loop that selects the best manifest currently only compares term and can pick a manifest from another branch; update both fallback loops in GetManifest and RefreshManifest to skip entries whose parsed branch_name does not match the current active branch before considering term. Concretely, after ParseManifestFileSuffix returns branch_name, add a check comparing branch_name to the repository's active branch variable (e.g., current active branch field used elsewhere in the class) and continue if it differs, then proceed to evaluate term/best_term and set selected_branch/ found as before; do this in the code paths around ParseManifestFileSuffix, selected_branch, best_term in both functions.
🧹 Nitpick comments (1)
include/common.h (1)
3-5: Consider the impact of adding<glog/logging.h>to a common header.Including
<glog/logging.h>in this widely-included header file adds a dependency on glog for every translation unit that includescommon.h. This is acceptable given the inline functions useLOG(WARNING), but be aware it may increase compile times across the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/common.h` around lines 3 - 5, Including <glog/logging.h> in include/common.h forces every TU to depend on glog; remove that include from common.h and instead place the glog include only where LOG(WARNING) is actually used—either in the source file that implements the inline functions or in a smaller dedicated header (e.g., logging_helpers.h) that only the files that call LOG include; alternatively refactor the inline functions in common.h to accept a logging callback or return error status so they no longer directly call LOG(WARNING). Identify and update the inline functions that reference LOG(WARNING) and move the include or refactor them accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/common.h`:
- Around line 525-547: ParseBranchTerm currently accumulates digits into a
uint64_t without guarding against overflow; add an overflow check before
updating term in the loop (in function ParseBranchTerm) by verifying that term
<= (UINT64_MAX - (c - '0')) / 10 (or equivalent) and return 0 (or another error
indicator) if the next digit would overflow; ensure the function still returns 0
on invalid or overflowing input so numeric overflow cannot silently wrap the
value.
In `@src/async_io_manager.cpp`:
- Around line 4862-4873: The manifest upload key is being built from global
GetActiveBranch()/ProcessTerm() instead of the file descriptor's metadata, so
change the manifest filename construction in SyncFile and SyncFiles to use
fd.Get()->branch_name_ and fd.Get()->term_ (or fd->term_) and call
BranchManifestFileName(fd.Get()->branch_name_, fd.Get()->term_) (or equivalent
accessors) instead of BranchManifestFileName(GetActiveBranch(), ProcessTerm());
similarly ensure any other upload key creation in these functions (where file_id
== LruFD::kManifest) uses fd's branch/term fields before calling UploadFile
(reference functions: SyncFile, SyncFiles, LruFD::kManifest,
BranchManifestFileName, UploadFile).
- Around line 4600-4616: BranchBaseNameExists currently maps errors from the
cloud list and parse steps to KvError::NotFound, which hides backend failures;
update the error handling so that when list_task.error_ != KvError::NoError you
return list_task.error_ (or an appropriate KvError reflecting the backend
failure) instead of KvError::NotFound, and likewise if
obj_store_.ParseListObjectsResponse(...) returns false return a non-NotFound
error (e.g. KvError::IOError or the parsing-specific KvError) so callers can
distinguish real NotFound from service/parse failures; modify the checks around
list_task and ParseListObjectsResponse in BranchBaseNameExists to propagate the
original/error-specific KvError rather than collapsing to KvError::NotFound.
- Around line 1053-1060: OpenOrCreateFD's fast-path currently reuses a cached
LruFD without validating it against the new branch context and term, risking IO
to the wrong inode; update the reuse check in IouringMgr::OpenOrCreateFD (and
the similar reuse site around the other occurrence) to compare the cached
LruFD's stored branch_name and term with the incoming branch_name and term and
only reuse the cached FD when both match; if they differ, treat it as a miss
(evict/close or reload the FD) so the function opens a fresh FD bound to the
correct branch/term.
- Around line 4746-4769: The loop submits cloud delete tasks (delete_tasks via
obj_store_.SubmitTask) and waits on each with current_task->WaitIo but never
checks each DeleteTask.error_, so the function always returns KvError::NoError
even if deletes failed; update the logic after the loop to iterate the
delete_tasks vector (or inspect each task right after WaitIo) and if any
task->error_ != KvError::NoError return that error (or the first non-NoError)
instead of KvError::NoError; keep the existing local cache cleanup
(IouringMgr::DeleteBranchFiles) but ensure the cloud-delete failure is
propagated by referencing the DeleteTask.error_ field from the submitted
delete_tasks (and functions like
SetKvTask/AcquireCloudSlot/obj_store_.SubmitTask remain unchanged).
---
Outside diff comments:
In `@src/async_io_manager.cpp`:
- Around line 3853-3875: The loop that selects the best manifest currently only
compares term and can pick a manifest from another branch; update both fallback
loops in GetManifest and RefreshManifest to skip entries whose parsed
branch_name does not match the current active branch before considering term.
Concretely, after ParseManifestFileSuffix returns branch_name, add a check
comparing branch_name to the repository's active branch variable (e.g., current
active branch field used elsewhere in the class) and continue if it differs,
then proceed to evaluate term/best_term and set selected_branch/ found as
before; do this in the code paths around ParseManifestFileSuffix,
selected_branch, best_term in both functions.
---
Nitpick comments:
In `@include/common.h`:
- Around line 3-5: Including <glog/logging.h> in include/common.h forces every
TU to depend on glog; remove that include from common.h and instead place the
glog include only where LOG(WARNING) is actually used—either in the source file
that implements the inline functions or in a smaller dedicated header (e.g.,
logging_helpers.h) that only the files that call LOG include; alternatively
refactor the inline functions in common.h to accept a logging callback or return
error status so they no longer directly call LOG(WARNING). Identify and update
the inline functions that reference LOG(WARNING) and move the include or
refactor them accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9f358e3-37bd-44bf-af80-62734f63472e
📒 Files selected for processing (4)
include/async_io_manager.hinclude/common.hsrc/async_io_manager.cppsrc/tasks/prewarm_task.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/async_io_manager.cpp`:
- Around line 1204-1207: The DLOG formatting in the GetBranchNameAndTerm debug
statements concatenates tokens (e.g., "tbl_id=" and "file_id=") without
separators; update the DLOG(INFO) << "GetBranchNameAndTerm, tbl_id=" << tbl_id
<< "file_id=" << file_id << " branch_name=" << branch_name << " term=" << term;
(and the similar DLOG calls later) to include a space or other separator between
fields (e.g., add " file_id=" or " tbl_id=" spacing) so output reads "tbl_id=XYZ
file_id=5" instead of "tbl_id=XYZfile_id=5".
- Around line 2291-2297: The function BranchBaseNameExists conflates directory
iteration errors with "not found"; when fs::directory_iterator(it, ec) sets ec,
log the error (keeping the existing message with ec.message()) but return an
I/O-specific error (e.g., KvError::IoError or the project's I/O error variant)
instead of KvError::NotFound so callers can distinguish filesystem failures from
a missing branch. Locate the error check around fs::directory_iterator and
replace the KvError::NotFound return with the appropriate KvError I/O variant,
preserving the LOG(WARNING) call and message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e90da19b-af47-4d37-83f0-306e2a1c0cf0
📒 Files selected for processing (1)
src/async_io_manager.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/async_io_manager.cpp (1)
3938-3989:⚠️ Potential issue | 🟠 MajorPromote selected manifest when branch differs, not only when term differs.
If
selected_term == process_termbutselected_branch != active_br, the code skips promotion and later opensmanifest_<active_br>_<process_term>, which may not exist locally.🔧 Suggested fix
- if (selected_term != process_term) + if (selected_term != process_term || selected_branch != active_br) { ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/async_io_manager.cpp` around lines 3938 - 3989, The promotion block currently only runs when selected_term != process_term; change the condition to also trigger when selected_branch != active_br (e.g., if (selected_term != process_term || selected_branch != active_br)) so the code will rename/upload the manifest when the branch differs even if the term matches; keep using BranchManifestFileName(selected_branch, selected_term) and BranchManifestFileName(active_br, process_term) for src/promoted names, perform Rename/Fdatasync/UploadFile as before, and ensure manifest_branch_term_ is updated to the promoted branch/term after the successful promotion.
♻️ Duplicate comments (8)
src/async_io_manager.cpp (8)
2311-2317:⚠️ Potential issue | 🟡 MinorDon’t collapse local directory iteration failure into
NotFound.
BranchBaseNameExistsreturnsKvError::NotFoundon iterator errors, which hides I/O failures as absence.🔧 Suggested fix
if (ec) { LOG(WARNING) << "BranchBaseNameExists: failed to iterate " << dir_path << ": " << ec.message(); - return KvError::NotFound; + return KvError::IoFail; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/async_io_manager.cpp` around lines 2311 - 2317, The BranchBaseNameExists function currently treats a directory_iterator construction error (ec) as KvError::NotFound, hiding real I/O failures; change the error handling so that when fs::directory_iterator it(dir_path, ec) sets ec you log the error and return a proper I/O-related error (e.g., KvError::IoError or a mapped error that preserves ec) instead of KvError::NotFound, keeping the LOG(WARNING) and including ec.message() to retain diagnostics; update any callers or tests if they rely on NotFound semantics.
4891-4900:⚠️ Potential issue | 🔴 CriticalBuild manifest upload keys from FD metadata, not global active branch/term.
Manifest upload naming in
SyncFile/SyncFilesstill usesGetActiveBranch()andProcessTerm(). If active branch/term changes between open and sync, uploads can target the wrong key.🔧 Suggested fix
- if (file_id == LruFD::kManifest) - { - filename = BranchManifestFileName(GetActiveBranch(), ProcessTerm()); - } + std::string_view branch = fd.Get()->branch_name_; + if (file_id == LruFD::kManifest) + { + filename = BranchManifestFileName(branch, term); + } else { - std::string_view branch = fd.Get()->branch_name_; filename = BranchDataFileName(file_id, branch, term); } ... - if (file_id == LruFD::kManifest) - { - filename = - BranchManifestFileName(GetActiveBranch(), ProcessTerm()); - } + std::string_view branch = fd.Get()->branch_name_; + if (file_id == LruFD::kManifest) + { + filename = BranchManifestFileName(branch, term); + } else { - std::string_view branch = fd.Get()->branch_name_; filename = BranchDataFileName(file_id, branch, term); }Also applies to: 4960-4970
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/async_io_manager.cpp` around lines 4891 - 4900, The manifest upload key is being built from global GetActiveBranch()/ProcessTerm() instead of the file-descriptor metadata, which can cause wrong-target uploads if branch/term change; update SyncFile/SyncFiles to use the FD's stored branch/term (e.g., use fd.Get()->branch_name_ and the FD's term stored on the FD object) when calling BranchManifestFileName/BranchDataFileName and when invoking UploadFile (replace GetActiveBranch()/ProcessTerm() with the per-FD values), and ensure every place around BranchManifestFileName, BranchDataFileName, and UploadFile (including the similar block at 4960-4970) reads branch/term from fd rather than global functions.
4627-4643:⚠️ Potential issue | 🟠 MajorPropagate cloud list/parse failures in branch base-name checks.
Returning
NotFoundfor list/parse failure conflates backend faults with true absence and can allow incorrect branch-create decisions.🔧 Suggested fix
if (list_task.error_ != KvError::NoError) { LOG(WARNING) << "BranchBaseNameExists: list failed for prefix " << prefix << ": " << ErrorString(list_task.error_); - return KvError::NotFound; + return list_task.error_; } ... if (!obj_store_.ParseListObjectsResponse(...)) { - return KvError::NotFound; + return KvError::Corrupted; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/async_io_manager.cpp` around lines 4627 - 4643, In BranchBaseNameExists, don't mask backend/list/parse failures as KvError::NotFound; instead propagate the actual failure: when list_task.error_ != KvError::NoError return list_task.error_ (not NotFound), and if obj_store_.ParseListObjectsResponse(...) fails return a suitable error that reflects a cloud/IO failure (e.g., KvError::IOError or propagate list_task.error_ if available) rather than KvError::NotFound so callers can distinguish real absence from backend errors.
1083-1133:⚠️ Potential issue | 🔴 CriticalValidate cached FD reuse in local mode too.
The branch/term mismatch guard is still gated by
cloud_mode. Local mode can also reuse a cached FD bound to a different branch inode.🔧 Suggested fix
- // Check for term or branch_name mismatch in cloud mode. - const bool cloud_mode = !options_->cloud_store_path.empty(); - if (cloud_mode && file_id != LruFD::kDirectory) + // Check for term or branch_name mismatch for all non-directory files. + if (file_id != LruFD::kDirectory) { bool mismatch = false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/async_io_manager.cpp` around lines 1083 - 1133, The current mismatch check is only performed when cloud_mode (options_->cloud_store_path not empty), so in local mode a cached FD bound to a different term/branch may be reused incorrectly; move or remove the cloud_mode gating so the term/branch comparisons (using term, branch_name, and cached values from lru_fd.Get()->term_ and lru_fd.Get()->branch_name_) run for any non-directory file_id (file_id != LruFD::kDirectory), and when mismatch is true call CloseDirect(old_idx), set lru_fd.Get()->reg_idx_ = -1 and return errors the same way (use ToKvError(res) on CloseDirect failure), otherwise unlock mu_ and return the cached lru_fd with KvError::NoError; ensure you still treat directory/file_id cases the same as before and preserve mu_.Unlock() calls around early returns.
4702-4721:⚠️ Potential issue | 🟠 MajorFail branch deletion when cloud list/delete operations fail.
The function currently logs list/parse failures and ignores individual delete task results, then returns success. This can leave branch objects behind while reporting
NoError.🔧 Suggested fix
+ KvError first_cloud_err = KvError::NoError; ... if (list_task.error_ != KvError::NoError) { LOG(WARNING) << "DeleteBranchFiles: list failed for prefix " << prefix << ": " << ErrorString(list_task.error_); - break; + if (first_cloud_err == KvError::NoError) + { + first_cloud_err = list_task.error_; + } + break; } ... if (!obj_store_.ParseListObjectsResponse(...)) { LOG(WARNING) << "DeleteBranchFiles: parse list response failed " << "for prefix " << prefix; - break; + if (first_cloud_err == KvError::NoError) + { + first_cloud_err = KvError::Corrupted; + } + break; } ... for (const std::string &path : paths_to_delete) { ... current_task->WaitIo(); + KvError del_err = delete_tasks.back().error_; + if (del_err != KvError::NoError && del_err != KvError::NotFound && + first_cloud_err == KvError::NoError) + { + first_cloud_err = del_err; + } } ... - return KvError::NoError; + return first_cloud_err;Also applies to: 4773-4796
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/async_io_manager.cpp` around lines 4702 - 4721, In DeleteBranchFiles, stop swallowing list/parse failures and individual delete errors: when list_task.error_ != KvError::NoError or obj_store_.ParseListObjectsResponse(...) returns false, return/propagate an error instead of just logging and breaking; also collect results from each delete task (the delete task objects created for batch_files) and if any delete fails, propagate a non-success KvError (or aggregate and return the first error) instead of returning success; update logic around list_task, ParseListObjectsResponse, batch_files, next_token and the delete-task handling so the function returns failure on any list/parse/delete error.
2429-2436:⚠️ Potential issue | 🟡 MinorSet fixed-file SQE flag when directory FD is registered.
The unlink SQEs ignore
dir_fd.FdPair().second; registered directory FDs needIOSQE_FIXED_FILE.🔧 Suggested fix
- int dir_fd_int = dir_fd.FdPair().first; + auto [dir_fd_int, dir_fd_registered] = dir_fd.FdPair(); for (const std::string &file_path : files_to_delete) { ... io_uring_sqe *unlink_sqe = GetSQE(UserDataType::BaseReq, &reqs.back()); + if (dir_fd_registered) + { + unlink_sqe->flags |= IOSQE_FIXED_FILE; + } io_uring_prep_unlinkat(unlink_sqe, dir_fd_int, file_path.c_str(), 0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/async_io_manager.cpp` around lines 2429 - 2436, The unlink SQEs currently ignore whether the directory FD is registered; after creating the SQE with GetSQE and calling io_uring_prep_unlinkat in the loop (references: dir_fd, dir_fd.FdPair(), GetSQE, UserDataType::BaseReq, io_uring_prep_unlinkat, unlink_sqe), check dir_fd.FdPair().second and if it indicates the FD is registered set the SQE fixed-file flag (set unlink_sqe->flags |= IOSQE_FIXED_FILE) immediately after preparing the SQE so the unlinkat uses the registered fixed file.
1224-1226:⚠️ Potential issue | 🟡 MinorFix concatenated debug fields in branch mapping logs.
Both DLOG statements miss a separator between
tbl_idandfile_id, which hurts observability when debugging branch/term mapping issues.💡 Suggested fix
- DLOG(INFO) << "GetBranchNameAndTerm, tbl_id=" << tbl_id - << "file_id=" << file_id << " branch_name=" << branch_name + DLOG(INFO) << "GetBranchNameAndTerm, tbl_id=" << tbl_id + << " file_id=" << file_id << " branch_name=" << branch_name << " term=" << term; ... - DLOG(INFO) << "SetBranchNameAndTerm, tbl_id=" << tbl_id - << "file_id=" << file_id << " branch_name=" << branch_name + DLOG(INFO) << "SetBranchNameAndTerm, tbl_id=" << tbl_id + << " file_id=" << file_id << " branch_name=" << branch_name << " term=" << term;Also applies to: 1249-1251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/async_io_manager.cpp` around lines 1224 - 1226, The DLOG statements in GetBranchNameAndTerm concatenate tbl_id and file_id without a separator, harming readability; update the DLOG(s) (the debug lines in GetBranchNameAndTerm and the similar DLOG at the later occurrence) to include a clear separator (e.g., add ", " or a labeled field like " file_id=") between tbl_id and file_id and ensure other fields (branch_name, term) remain clearly separated so the log reads unambiguously.
2412-2415:⚠️ Potential issue | 🟡 MinorBound
max_termbefore reserve/loop in local branch deletion.If
CURRENT_TERMis corrupted to a huge value,reserve(max_term + 2)and the loop can explode memory/CPU.🛡️ Suggested fix
+ constexpr uint64_t kMaxReasonableTerm = 1000000; + if (max_term > kMaxReasonableTerm) + { + LOG(WARNING) << "DeleteBranchFiles: max_term " << max_term + << " exceeds sanity limit, capping to " + << kMaxReasonableTerm; + max_term = kMaxReasonableTerm; + } std::vector<std::string> files_to_delete; files_to_delete.reserve(max_term + 2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/async_io_manager.cpp` around lines 2412 - 2415, Clamp/bound max_term to a safe upper limit before calling files_to_delete.reserve(...) and before the for loop to avoid unbounded allocation/iteration if CURRENT_TERM is corrupted. Replace direct use of max_term in reserve and the loop (the vector files_to_delete and the for (uint64_t t = 0; t <= max_term; ++t) construct) with a bounded value (e.g., bounded_max = std::min(max_term, SAFE_LIMIT)), use bounded_max for reserve and the loop, and add a warning/log and a fallback/error path when you detect truncation so corruption is recorded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/async_io_manager.cpp`:
- Around line 4124-4133: RefreshManifest wrongly skips entries when
ParseFileName(name) != FileNameManifest; instead allow suffix-only entries by
trying ParseManifestFileSuffix on the raw filename when ParseFileName doesn't
return FileNameManifest. Concretely, in RefreshManifest change the logic around
ParseFileName/ParseManifestFileSuffix so that if type == FileNameManifest you
call ParseManifestFileSuffix(suffix, branch_name, term, ts), otherwise call
ParseManifestFileSuffix(name, branch_name, term, ts) (or otherwise treat the
whole name as the suffix) and proceed only if ParseManifestFileSuffix succeeds;
keep the rest of the loop (branch_name, term, ts handling) unchanged. Ensure you
reference the functions RefreshManifest, ParseFileName, FileNameManifest and
ParseManifestFileSuffix when making the change.
---
Outside diff comments:
In `@src/async_io_manager.cpp`:
- Around line 3938-3989: The promotion block currently only runs when
selected_term != process_term; change the condition to also trigger when
selected_branch != active_br (e.g., if (selected_term != process_term ||
selected_branch != active_br)) so the code will rename/upload the manifest when
the branch differs even if the term matches; keep using
BranchManifestFileName(selected_branch, selected_term) and
BranchManifestFileName(active_br, process_term) for src/promoted names, perform
Rename/Fdatasync/UploadFile as before, and ensure manifest_branch_term_ is
updated to the promoted branch/term after the successful promotion.
---
Duplicate comments:
In `@src/async_io_manager.cpp`:
- Around line 2311-2317: The BranchBaseNameExists function currently treats a
directory_iterator construction error (ec) as KvError::NotFound, hiding real I/O
failures; change the error handling so that when fs::directory_iterator
it(dir_path, ec) sets ec you log the error and return a proper I/O-related error
(e.g., KvError::IoError or a mapped error that preserves ec) instead of
KvError::NotFound, keeping the LOG(WARNING) and including ec.message() to retain
diagnostics; update any callers or tests if they rely on NotFound semantics.
- Around line 4891-4900: The manifest upload key is being built from global
GetActiveBranch()/ProcessTerm() instead of the file-descriptor metadata, which
can cause wrong-target uploads if branch/term change; update SyncFile/SyncFiles
to use the FD's stored branch/term (e.g., use fd.Get()->branch_name_ and the
FD's term stored on the FD object) when calling
BranchManifestFileName/BranchDataFileName and when invoking UploadFile (replace
GetActiveBranch()/ProcessTerm() with the per-FD values), and ensure every place
around BranchManifestFileName, BranchDataFileName, and UploadFile (including the
similar block at 4960-4970) reads branch/term from fd rather than global
functions.
- Around line 4627-4643: In BranchBaseNameExists, don't mask backend/list/parse
failures as KvError::NotFound; instead propagate the actual failure: when
list_task.error_ != KvError::NoError return list_task.error_ (not NotFound), and
if obj_store_.ParseListObjectsResponse(...) fails return a suitable error that
reflects a cloud/IO failure (e.g., KvError::IOError or propagate
list_task.error_ if available) rather than KvError::NotFound so callers can
distinguish real absence from backend errors.
- Around line 1083-1133: The current mismatch check is only performed when
cloud_mode (options_->cloud_store_path not empty), so in local mode a cached FD
bound to a different term/branch may be reused incorrectly; move or remove the
cloud_mode gating so the term/branch comparisons (using term, branch_name, and
cached values from lru_fd.Get()->term_ and lru_fd.Get()->branch_name_) run for
any non-directory file_id (file_id != LruFD::kDirectory), and when mismatch is
true call CloseDirect(old_idx), set lru_fd.Get()->reg_idx_ = -1 and return
errors the same way (use ToKvError(res) on CloseDirect failure), otherwise
unlock mu_ and return the cached lru_fd with KvError::NoError; ensure you still
treat directory/file_id cases the same as before and preserve mu_.Unlock() calls
around early returns.
- Around line 4702-4721: In DeleteBranchFiles, stop swallowing list/parse
failures and individual delete errors: when list_task.error_ != KvError::NoError
or obj_store_.ParseListObjectsResponse(...) returns false, return/propagate an
error instead of just logging and breaking; also collect results from each
delete task (the delete task objects created for batch_files) and if any delete
fails, propagate a non-success KvError (or aggregate and return the first error)
instead of returning success; update logic around list_task,
ParseListObjectsResponse, batch_files, next_token and the delete-task handling
so the function returns failure on any list/parse/delete error.
- Around line 2429-2436: The unlink SQEs currently ignore whether the directory
FD is registered; after creating the SQE with GetSQE and calling
io_uring_prep_unlinkat in the loop (references: dir_fd, dir_fd.FdPair(), GetSQE,
UserDataType::BaseReq, io_uring_prep_unlinkat, unlink_sqe), check
dir_fd.FdPair().second and if it indicates the FD is registered set the SQE
fixed-file flag (set unlink_sqe->flags |= IOSQE_FIXED_FILE) immediately after
preparing the SQE so the unlinkat uses the registered fixed file.
- Around line 1224-1226: The DLOG statements in GetBranchNameAndTerm concatenate
tbl_id and file_id without a separator, harming readability; update the DLOG(s)
(the debug lines in GetBranchNameAndTerm and the similar DLOG at the later
occurrence) to include a clear separator (e.g., add ", " or a labeled field like
" file_id=") between tbl_id and file_id and ensure other fields (branch_name,
term) remain clearly separated so the log reads unambiguously.
- Around line 2412-2415: Clamp/bound max_term to a safe upper limit before
calling files_to_delete.reserve(...) and before the for loop to avoid unbounded
allocation/iteration if CURRENT_TERM is corrupted. Replace direct use of
max_term in reserve and the loop (the vector files_to_delete and the for
(uint64_t t = 0; t <= max_term; ++t) construct) with a bounded value (e.g.,
bounded_max = std::min(max_term, SAFE_LIMIT)), use bounded_max for reserve and
the loop, and add a warning/log and a fallback/error path when you detect
truncation so corruption is recorded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5eaa1e59-9f2d-49f4-860a-09223bf0ddc3
📒 Files selected for processing (1)
src/async_io_manager.cpp
githubzilla
force-pushed
the
branch_support
branch
from
6da5767 to
769ef8e
Compare
Fix regressions from manual merge: restore branch_design API usage - async_io_manager.cpp: remove download_to_exist block from DownloadFile body; fix OpenOrCreateFD to 6-arg form in DownloadFile and WriteFile; fix BranchBaseNameExists to pass store_path_lut to StorePath - async_io_manager.h: restore DownloadFile signature with string_view branch_name (not bool download_to_exist) - eloq_store.h: restore missing comma after GlobalReopen in RequestType - index_page_manager.cpp: fix InstallExternalSnapshot to use GetBranchNameAndTerm + DownloadFile(branch_name) and SetBranchFileMapping instead of old SetFileIdTermMapping / file_id_term_mapping_ - tests/cloud.cpp: replace ArchiveName/ManifestFileName with Branch* forms and fix Start() -> Start(MainBranchName, 0) Fix CURRENT_TERM cloud upload to use per-branch suffix All four CloudStoreMgr term-file operations (ReadTermFile, UpsertTermFile, CasCreateTermFile, CasUpdateTermFileWithEtag) now use BranchCurrentTermFileName(branch_name) instead of the bare CurrentTermFileName constant, producing CURRENT_TERM.<branch> (e.g. CURRENT_TERM.main) in cloud object storage. GetManifest passes active_br to UpsertTermFile; ExecuteCloudGC passes cloud_mgr->GetActiveBranch() to ReadTermFile. All function signatures in the header are updated with a defaulted branch_name parameter. gc test updated: CheckCloudPartitionExists now expects CURRENT_TERM.main (via BranchCurrentTermFileName(MainBranchName)) instead of the bare CURRENT_TERM. fix: re-enqueue CreateBranch/DeleteBranch when task pool is exhausted When max_write_concurrency is reached, GetBackgroundWrite returns nullptr and ProcessReq returns false. For CreateBranch and DeleteBranch requests, the return value was previously ignored, causing the request to be silently dropped. This meant GlobalCreateBranchRequest's pending_ counter never reached 0, and the branch creation operation hung indefinitely. Fix: check ProcessReq's return value and re-enqueue the request into requests_ so it is retried on the next WorkOneRound iteration. fix: treat empty/stub partition as success in CreateBranch Partitions with no manifest yet (FindRoot returns NotFound) or a null mapper (stub partition with no data) do not need a branch manifest. Previously these returned KvError::NotFound which propagated as an error and caused all 14332 partitions to fail. Now both cases return KvError::NoError so CreateBranch succeeds for empty partitions. fix: treat AlreadyExists as success in HandleGlobalCreateBranchRequest for idempotency fix: fallback to active branch in OnDataFileSealed when file_id mapping missing CreateBranch should fail if branch already exist Fix build error Fix clang-format Fix clang-format fix: port 204ff65 and 3a6f964 to branch_support - RestoreFilesForTable: add term field to CachedFileInfo and select max data file by (term, file_id) order instead of file_id only, so the correct latest file is removed on reopen regardless of branch naming (from eloqstore commit 3a6f964) - DownloadFile: restore atomic write via tmp+rename so a crash mid-download never leaves a corrupt file (regression from 94c0390 removed in 878485e); add download_to_exist parameter that renames the existing file to .tmp before downloading, enabling safe in-place replacement of an already cached data file (from eloqstore commit 204ff65) - InstallExternalSnapshot: pass download_to_exist=true when re-downloading the max data file on reopen so the local copy is atomically replaced refactor: move branch_name before term in DownloadFile signature - Reorder parameters so branch_name comes before term, matching the convention used by helper functions like BranchManifestFileName and BranchDataFileName - Update declaration, implementation, and all 5 call sites accordingly Fix manifest deletion bug in file_gc Rename DownloadArchiveFile to ReadCloudFile and use salted temp filename to avoid deleting manifest files currently in use. fix: set branch_name and term on manifest FD and normalize directory FD params - Expand OpenOrCreateFD post-open assignment to cover manifest FDs (file_id != kDirectory) so branch_name_ and term_ are correctly recorded, not just for data files. - Update CloudStoreMgr::CloseFile to read branch/term from the FD for manifest files instead of using GetActiveBranch()/ProcessTerm(). - Normalize all OpenFD/OpenOrCreateFD calls with kDirectory to pass empty branch_name and term=0, since directory FDs are not associated with any specific branch or term. Remove redundant GetFileIdTerm call in InstallExternalSnapshot GetFileIdTerm internally calls GetBranchNameAndTerm, so calling both results in a redundant lookup. Use a single GetBranchNameAndTerm call and fall back to MainBranchName and ProcessTerm() when no mapping exists. Remove GetFileIdTerm and use ProcessTerm() directly in write paths WritePage and SubmitMergedWrite only need the current process term, not a looked-up term from the branch file mapping. Replace GetFileIdTerm calls with ProcessTerm() in both functions. In write_task.cpp, replace the GetFileIdTerm existence check with GetBranchNameAndTerm, which is the underlying lookup GetFileIdTerm was wrapping. With no remaining callers, remove the GetFileIdTerm method entirely (virtual base declaration, IouringMgr override, and definition). Intern branch name strings in LruFD to avoid per-FD heap allocations Change LruFD::branch_name_ from std::string to std::string_view backed by an absl::node_hash_set<std::string> pool in IouringMgr. The pool provides pointer-stable storage so string_views remain valid for the lifetime of the manager. InternBranchName() uses find-then-emplace to avoid allocating a std::string on every lookup hit. Add debug log Remove DataFileName function, using BranchDataFileName instead Refine logs Fix ReadFilePrefix race: read from cached FD instead of opening by path When uploading a manifest, ReadFilePrefix previously opened a new FD by path to read the file prefix. A concurrent rename() (from prewarm download or GetManifest) could replace the file on disk between the write and the prefix read, causing an unexpected EOF. Fix by threading the cached FD (inode-based) from SyncFile/SyncFiles through UploadFile to ReadFilePrefix. The cached FD references the inode directly and is immune to path-level replacement via rename(). Add debug log Fix OpenOrCreateFD reusing cached FD with stale branch_name When prewarm downloads a file under one branch, it caches an LruFD with that branch_name. If the write path later needs the same file_id under a different branch with term=0, OpenOrCreateFD() previously skipped all mismatch checks (the term!=0 guard excluded it) and reused the stale FD. This caused SyncFile to construct an upload filename with the wrong branch, triggering 'WriteTask upload state filename mismatch' and startup failure. Add branch_name mismatch detection alongside the existing term check, evaluated regardless of term value, so stale FDs from prewarm are closed and reopened with the correct branch. debug log Use active branch when max_file_id not found when install external snapshot Branch name should not be null when compare cached fd and file to open Remove not used funtion of FilePageAllocatore::SetCurrentFileId Pass term as parameter to CreateArchive instead of calling ProcessTerm internally Remove default parameter values from OpenFD and OpenOrCreateFD Remove default values and reorder branch_name parameter in CloudStoreMgr private methods Remove default parameter values from DownloadFile (branch_name, term, download_to_exist) and ReadTermFile (branch_name). Reorder parameters in UpsertTermFile, CasCreateTermFile, and CasUpdateTermFileWithEtag so that branch_name comes right after tbl_id, consistent with the convention established in other functions. Update all call sites accordingly. Use system_clock instead of high_resolution_clock for branch name salt high_resolution_clock is unnecessarily expensive for generating a non-cryptographic salt value. system_clock is cheaper and sufficient. Remove BranchBaseNameExists duplicate-name check and dead code Salted branch names guarantee uniqueness, making the per-partition BranchBaseNameExists check unnecessary and too expensive to run across all table partitions. Remove the check from CreateBranch along with the now-unused BranchBaseNameExists virtual function (base + 3 overrides) and UnsaltBranchName helper. Remove duplicate branch name test cases These tests expected AlreadyExists when creating a branch with the same name twice. The BranchBaseNameExists check was removed in the previous commit, so duplicate user-visible branch names are no longer rejected. Remove unused BranchManifestExists and BranchCurrentTermExists Refactor ParseBranchTerm to return bool and propagate parse errors Change ParseBranchTerm from returning uint64_t (0 on error) to returning bool with an output parameter, so callers can distinguish a valid term 0 from a parse failure. Add overflow guard for the digit accumulation. Update the call site in DeleteBranchFiles to return KvError::IoFail when CURRENT_TERM content is invalid instead of silently falling back to 0. Remove unused parent_branch_ from GlobalCreateBranchRequest The parent_branch_ member, its getter, and the corresponding SetArgs parameter were never read by HandleGlobalCreateBranchRequest or any other code. Simplify SetArgs to accept only branch_name and update all call sites. Add back comments for GetMetricsMeter Add AlreadyExists to FFI error enums and converters KvError::AlreadyExists was defined in error.h but missing from the C FFI enum, the kv_error_to_c() converter, and all Rust error definitions. This caused AlreadyExists to fall through to CEloqStoreStatus_InvalidArgs via the default case. Add the variant across the full FFI stack: C header, C++ converter, eloqstore-sys bindings, and eloqstore Rust wrapper. Call SetBranchFileMapping unconditionally to clear stale mappings Both call sites in index_page_manager.cpp guarded SetBranchFileMapping behind an empty check, which left stale branch file mappings when a refreshed manifest had no ranges. Remove the guards so the mapping is always overwritten, correctly clearing old entries when the new manifest has an empty file_ranges. Fix some review comment Roll back orphaned manifest when WriteBranchCurrentTerm fails in CreateBranch If WriteBranchManifest succeeds but WriteBranchCurrentTerm fails, the branch is left half-created with an orphaned manifest file and no CURRENT_TERM marker. Add a DeleteBranchFiles rollback call to clean up the manifest so the partition does not accumulate stale files. Add debug log Add debug log Remove unused file_id_term_mapping_dirty_ variable from WriteTask Rename term_buf Remove unused FileIdTermMapping type and related dead code Fix missing store_path_lut argument in DeleteOldArchives DeleteOldArchives called tbl_id.StorePath(store_path) without the store_path_lut, relying on the default empty span. This caused an assertion failure in StorePathIndex when the LUT was expected to be non-empty. Pass the LUT explicitly, matching all other StorePath call sites. Register manifest_payload test in CMakeLists.txt Value-initialize BranchFileRange and BranchManifestMetadata members term and max_file_id were left uninitialized in both structs, leading to undefined values when the deserializer returns early on truncated input. Add BranchManifestMetadata serialization tests Test roundtrip with non-empty file_ranges, empty file_ranges, zero term, large values (UINT64_MAX), truncated/empty input, and manifest snapshot integration with non-empty file_ranges. Assert Start() return value in cloud tests Assert Start() return value in persist and manifest tests Filter all CURRENT_TERM files in CheckCloudPartitionExists Change DeserializeBranchManifestMetadata to return bool with out parameter Check Start() return value in benchmarks and concurrent_test Propagate errors from AugmentRetainedFilesFromBranchManifests instead of skipping ProcessOneManifest now returns KvError instead of void. AugmentRetainedFilesFromBranchManifests returns errors instead of continuing with an incomplete retained-files set, which could cause incorrect file deletion (data loss). Both callers (ExecuteLocalGC, ExecuteCloudGC) check the return value and abort the GC cycle on failure. Use GetActiveBranch() for manifest selection in cloud GC In DeleteUnreferencedCloudFiles, the active branch's manifest was identified by scanning for the first manifest whose term matches process_term. This could theoretically select the wrong branch if multiple branches share the same term value (e.g. term 0). Use cloud_mgr->GetActiveBranch() directly instead, which is authoritative. Also fixes log messages that incorrectly referenced ExecuteLocalGC instead of ExecuteCloudGC, and simplifies the superseded-manifest pruning block by removing the redundant term-based branch lookup. Update comment for removing current branch term Remove incorrect comment Set IOSQE_FIXED_FILE for registered dir FD in DeleteBranchFiles The unlink loop in DeleteBranchFiles used the directory FD from OpenFD but never checked whether it was a registered file index. When the FD is registered, io_uring requires IOSQE_FIXED_FILE to interpret the fd field as a registered index. This matches the pattern used by UnlinkAt() at the same call site. Check cloud delete errors in CloudStoreMgr::DeleteBranchFiles Route CreateBranch/DeleteBranch through pending write queue Compact data files before CreateBranch in append mode
githubzilla
force-pushed
the
branch_support
branch
from
fcc7849 to
337846c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Here are some reminders before you submit the pull request
fixes eloqdb/eloqstore#issue_idctest --test-dir build/tests/Summary by CodeRabbit
New Features
Refactor
Tests
Chores